Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: addressability #2731

Open
wants to merge 41 commits into
base: master
Choose a base branch
from
Open

Conversation

deelawn
Copy link
Contributor

@deelawn deelawn commented Aug 27, 2024

Closes #2299.

This PR enforces addressability rules from the go spec. Basically, you can't take references of values that don't have addresses, which makes sense. I think it's important to have the VM's behavior match Go's in this regard. If this isn't in place before we launch and we decide to do it later, we run the risk of breaking realms with code that doesn't adhere to the addressability rules. Omitting addressability rules may also have unforeseen future consequences when we decide to make the VM compatible with newer Go language features.

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Aug 27, 2024
Copy link

codecov bot commented Aug 27, 2024

Codecov Report

Attention: Patch coverage is 61.45833% with 37 lines in your changes missing coverage. Please review.

Project coverage is 61.06%. Comparing base (9897b66) to head (b3d9f9a).
Report is 30 commits behind head on master.

Files with missing lines Patch % Lines
gnovm/pkg/gnolang/nodes.go 49.31% 35 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2731      +/-   ##
==========================================
+ Coverage   60.87%   61.06%   +0.19%     
==========================================
  Files         563      564       +1     
  Lines       75193    75448     +255     
==========================================
+ Hits        45770    46071     +301     
+ Misses      26055    25988      -67     
- Partials     3368     3389      +21     
Flag Coverage Δ
contribs/gnodev 61.46% <ø> (ø)
contribs/gnofaucet 14.46% <ø> (-0.86%) ⬇️
gno.land 67.92% <ø> (+0.75%) ⬆️
gnovm 65.91% <61.45%> (+0.27%) ⬆️
misc/genstd 80.54% <ø> (ø)
misc/logos 20.23% <ø> (ø)
tm2 62.03% <ø> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ltzmaxwell
Copy link
Contributor

start looking, this seems to be a missing case:

package main

import "fmt"

type MyStruct struct {
	Mp *int
}

func makeT() MyStruct {
	x := 10
	return MyStruct{Mp: &x}
}

func main() {
	p := &makeT().Mp // This would cause an error because you cannot take the address of a struct field when the struct is a direct return value of a function.
	fmt.Println(p)
}

baseType := baseOf(ft.Results[0].Type)
switch baseType.(type) {
case *PointerType, *SliceType:
n.IsAddressable = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is out of scope, but the error should be about assignment mismatch.

package main

func foo() ([]int, []string) {

	return []int{1, 2, 3}, []string{"a", "b", "c"}
}

func main() {

	r1 := &foo()

}

// Error:
// cannot take address of foo<VPBlock(3,0)>():

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is actually in scope; you're right that in this case it doesn't make sense for the addressability error message to supersede the results mismatch. In fact, the addressability message doesn't make sense because it is trying to take the address of multiple values. Addressed this here: 3f4f1b6

@ltzmaxwell
Copy link
Contributor

ltzmaxwell commented Aug 29, 2024

another one out of scope too: 😆

package main

type MyInt int

func main() {

	_ = &MyInt
	_ = MyInt

}

@deelawn deelawn marked this pull request as ready for review August 29, 2024 15:52
gnovm/pkg/gnolang/nodes.go Outdated Show resolved Hide resolved
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bunch of proposed reorgs, overall implementation looks good, good job 🎉

gnovm/pkg/gnolang/addressability.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/nodes.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/nodes.go Outdated Show resolved Hide resolved
Args Exprs // function arguments, if any.
Varg bool // if true, final arg is variadic.
NumArgs int // len(Args) or len(Args[0].Results)
Addressability Addressability
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the cases like this where the addressability is assigned during preprocess, what do you think about moving Addressability as an attribute?

The reason why I say this is that we currently have a bunch of information which is not actually persisted, but recovered by using Preprocess. The Expr types here contain in their struct only information which comes directly out of the AST representation; not from preprocessing. I'm not entirely sure of why it's designed this way; but I think for consistency we should maintain the split between "preprocessed information" in Attributes, and AST information in the struct themselves.

If addressability is an attribute, I also wonder if there's a way to remove the specific type and turn this into Addressable() bool instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is a good way to make an Addressable() bool method due to reasons I outlined in responses to other comments -- there are one or more cases where addressability is not applicable so the preprocessor should handle execution as usual. That being said, while many other attributes apply only exist some of the time, the Addressability attribute exists for all addressable expressions -- everywhere it appears. For ease of access, I'd prefer not to make this an attribute.

gnovm/pkg/gnolang/nodes.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/nodes.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/nodes.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/preprocess.go Outdated Show resolved Hide resolved
gnovm/pkg/gnolang/preprocess.go Show resolved Hide resolved
@@ -1671,6 +1768,30 @@ func (sb *StaticBlock) GetIsConst(store Store, n Name) bool {
}
}

func (sb *StaticBlock) getAt(store Store, path ValuePath) *StaticBlock {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func (sb *StaticBlock) getAt(store Store, path ValuePath) *StaticBlock {
func (sb *StaticBlock) GetBlockNodeForPath(store Store, path ValuePath) *StaticBlock {

gnovm/pkg/gnolang/nodes.go Outdated Show resolved Hide resolved
Copy link
Member

@omarsy omarsy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can fix this issue #2840 on this PR, or it is out of scope ?
(We should prohibit assignments to non-addressable attributes.)

deelawn and others added 2 commits September 25, 2024 10:07
Co-authored-by: ltzmaxwell <ltz.maxwell@gmail.com>
@thehowl thehowl self-assigned this Oct 15, 2024
thehowl pushed a commit that referenced this pull request Oct 21, 2024
…lue (#2848)

First part of the fix: #2836

The second part is addressed here:
#2731
@thehowl thehowl assigned ltzmaxwell and unassigned deelawn and thehowl Oct 21, 2024
@thehowl
Copy link
Member

thehowl commented Oct 21, 2024

@ltzmaxwell to finish the work on this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: In Progress
Status: In Review
Development

Successfully merging this pull request may close these issues.

[bug/gnovm] arrays returned by functions should not be addressable
5 participants